tests/simulator/test_advance.py (3)

290-291: Remove unused matplotlib.pyplot import.

The matplotlib.pyplot import is not used in the current code. Only Rectangle from matplotlib.patches is needed.

-                        import matplotlib.pyplot as plt
-                        from matplotlib.patches import Rectangle
+                        from matplotlib.patches import Rectangle

---

293-300: Improve variable naming for better readability.

The variable l in the list comprehension is ambiguous. Use more descriptive names for better code clarity.

                        def makerec(lower, upper, dl, fc="none", ec="g", lw=1, ls="-"):
                            origin = (lower[0] * dl[0], lower[1] * dl[1])
                            sizex, sizey = [
-                               (u - l) * d for u, l, d in zip(upper, lower, dl)
+                               (u - low) * d for u, low, d in zip(upper, lower, dl)
                            ]
                            return Rectangle(
                                origin, sizex, sizey, fc=fc, ec=ec, ls=ls, lw=lw
                            )

---

302-330: Excellent debugging enhancement with minor suggestions.

The comprehensive visualization code provides valuable debugging context for field overlap failures. The implementation correctly highlights failing overlap boxes and generates descriptive plots.

Consider these improvements:

1. Extract visualization logic: Consider moving this debugging code to a separate method to improve readability and reusability.

2. Add dimensionality check: The code assumes 2D data when accessing pd1.layout.dl. Add a check for the data dimensionality:

if len(pd1.layout.dl) >= 2:  # Ensure 2D or higher
    # existing plotting code
else:
    print("Visualization skipped: only supports 2D+ data")

3. Optional conditional execution: Consider adding a flag to control when expensive plotting occurs:

if self.debug_plots:  # or environment variable check
    # plotting code

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

tests/simulator/test_advance.py (1)

353-355: Consider making error handling behavior configurable.

Commenting out the error re-throwing and diff_boxes return significantly changes the test behavior from fail-fast to log-and-continue. While this provides better debugging information, it could mask legitimate test failures.

Consider making this behavior configurable:

-                        # if self.rethrow_:
-                        #     raise e
-                        # return diff_boxes(slice1, slice2, box)
+                        if getattr(self, 'rethrow_', True):
+                            raise e
+                        return diff_boxes(slice1, slice2, box)

This allows debugging mode when needed while preserving the original test semantics by default. You could set self.rethrow_ = False in specific test cases or via environment variables when debugging is required.

tests/simulator/test_advance.py (3)

290-292: Fix unused import and improve import organization.

The matplotlib.pyplot import is unused since the code uses datahier.plot() method instead. Additionally, consider moving these imports to the top of the method or using conditional imports.

-        import matplotlib.pyplot as plt
-        from matplotlib.patches import Rectangle
+        from matplotlib.patches import Rectangle

---

298-307: Fix ambiguous variable name in lambda function.

The lambda function uses an ambiguous variable name l which reduces code readability.

-                            def makerec(
-                                lower, upper, dl, fc="none", ec="g", lw=1, ls="-"
-                            ):
-                                origin = (lower[0] * dl[0], lower[1] * dl[1])
-                                sizex, sizey = [
-                                    (u - l) * d for u, l, d in zip(upper, lower, dl)
-                                ]
-                                return Rectangle(
-                                    origin, sizex, sizey, fc=fc, ec=ec, ls=ls, lw=lw
-                                )
+                            def makerec(
+                                lower, upper, dl, fc="none", ec="g", lw=1, ls="-"
+                            ):
+                                origin = (lower[0] * dl[0], lower[1] * dl[1])
+                                sizex, sizey = [
+                                    (upper_val - lower_val) * delta for upper_val, lower_val, delta in zip(upper, lower, dl)
+                                ]
+                                return Rectangle(
+                                    origin, sizex, sizey, fc=fc, ec=ec, ls=ls, lw=lw
+                                )

---

293-342: Consider adding 1D and 3D debugging support.

The debugging visualization code only handles 2D cases. Consider adding support for other dimensions or at least documenting this limitation.

                        if box.ndim == 2:
                            failed_i, failed_j = np.where(
                                np.abs(slice1 - slice2) > 5.5e-15
                            )
+                        elif box.ndim == 1:
+                            failed_i = np.where(np.abs(slice1 - slice2) > 5.5e-15)[0]
+                            print(f"1D debugging: failed indices: {failed_i}")
+                        else:
+                            print(f"Debugging visualization not implemented for {box.ndim}D")

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

tests/simulator/test_advance.py (1)

359-361: Consider making error handling behavior configurable.

Commenting out the error re-throwing and diff_boxes return significantly changes the test behavior from fail-fast to log-and-continue. While this provides better debugging information, it could mask legitimate test failures.

Consider making this behavior configurable:

-                        # if self.rethrow_:
-                        #     raise e
-                        # return diff_boxes(slice1, slice2, box)
+                        if getattr(self, 'rethrow_', True):
+                            raise e
+                        return diff_boxes(slice1, slice2, box)

This allows debugging mode when needed while preserving the original test semantics by default. You could set self.rethrow_ = False in specific test cases or via environment variables when debugging is required.

tests/simulator/test_advance.py (1)

301-310: Fix ambiguous variable name and improve code clarity.

The variable name l in the list comprehension is ambiguous and flagged by static analysis. Also consider adding explicit handling for 3D cases.

Apply this diff to fix the ambiguous variable name:

-                                sizex, sizey = [
-                                    (u - l) * d for u, l, d in zip(upper, lower, dl)
-                                ]
+                                sizex, sizey = [
+                                    (u - low) * d for u, low, d in zip(upper, lower, dl)
+                                ]

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

tests/simulator/test_advance.py (1)

394-396: Consider making error handling behavior configurable.

The commented-out error re-throwing significantly changes test behavior from fail-fast to log-and-continue, potentially masking legitimate test failures. This matches a previous review comment about the same issue.

While the enhanced debugging information is valuable, consider making this behavior configurable:

-                        # if self.rethrow_:
-                        #     raise e
-                        # return diff_boxes(slice1, slice2, box)
+                        if getattr(self, 'rethrow_', True):
+                            raise e
+                        return diff_boxes(slice1, slice2, box)

This allows debugging mode when needed while preserving the original test semantics by default.

src/amr/data/field/field_variable_fill_pattern.hpp (1)

107-120: Consider optimizing the dynamic casting and improve code clarity.

The conditional logic correctly implements the intended behavior, but there are some concerns:

1. Performance: Two separate dynamic_cast operations are performed when overwrite_interior_ is false, which could be expensive in performance-critical paths.

2. Code clarity: The variable names geom, basic_overlap, and casted could be more descriptive.

Consider this refactor to optimize performance and improve readability:

-        auto geom = pdf.getBoxGeometry(patch_box);
-        auto basic_overlap
-            = pdf.getBoxGeometry(patch_box)->setUpOverlap(overlap_boxes, transformation);
+        auto geom = pdf.getBoxGeometry(patch_box);
+        auto basic_overlap = geom->setUpOverlap(overlap_boxes, transformation);

         if (overwrite_interior_)
             return basic_overlap;

-        auto& overlap         = dynamic_cast<FieldOverlap const&>(*basic_overlap);
-        auto destinationBoxes = overlap.getDestinationBoxContainer();
-        auto& casted          = dynamic_cast<FieldGeometryBase<dimension> const&>(*geom);
-        destinationBoxes.removeIntersections(casted.interiorFieldBox());
+        auto& field_overlap = dynamic_cast<FieldOverlap const&>(*basic_overlap);
+        auto destinationBoxes = field_overlap.getDestinationBoxContainer();
+        auto& field_geometry = dynamic_cast<FieldGeometryBase<dimension> const&>(*geom);
+        destinationBoxes.removeIntersections(field_geometry.interiorFieldBox());

-        return std::make_shared<FieldOverlap>(destinationBoxes, overlap.getTransformation());
+        return std::make_shared<FieldOverlap>(destinationBoxes, field_overlap.getTransformation());

This eliminates the redundant getBoxGeometry call and uses more descriptive variable names.

tests/simulator/test_advance.py (2)

290-292: Remove unused matplotlib.pyplot import.

The matplotlib.pyplot import is unused according to static analysis. The code only uses Rectangle from matplotlib.patches.

-                        import matplotlib.pyplot as plt
-                        from matplotlib.patches import Rectangle
+                        from matplotlib.patches import Rectangle

---

293-380: Consider extracting visualization logic to a helper method.

The debugging visualization code is quite extensive and could benefit from being extracted to a separate helper method to improve maintainability and readability of the main test method.

Consider creating a helper method like _create_overlap_failure_plots(self, pd1, pd2, box, failed_indices, ilvl, datahier) to encapsulate this visualization logic. This would:

1. Reduce the complexity of the main test method
2. Make the visualization code reusable
3. Improve code organization and readability
4. Make the test method easier to understand and maintain

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

tests/simulator/test_advance.py (2)

402-404: Consider making error handling behavior configurable.

Commenting out the error re-throwing and diff_boxes return significantly changes the test behavior from fail-fast to log-and-continue. While this provides better debugging information, it could mask legitimate test failures.

---

301-310: Fix ambiguous variable name and improve function clarity.

The makerec function uses ambiguous variable names like l which violates naming conventions and reduces readability.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)

407-426: Document the NaN initialization strategy.

The ghost layer NaN initialization appears to be implementing the "copy before refinement" behavior. While the implementation looks correct, it would benefit from a comment explaining why ghost values are set to NaN before refinement.

Add a comment before the loop:

// Initialize ghost layer values to NaN before refinement.
// This ensures we can distinguish between values that were copied
// vs. refined when fine_boundary_represents_var is false.
for (auto& comp : E)
{
    // ... existing code
}

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

tests/simulator/test_advance.py (2)

301-307: Fix ambiguous variable name in list comprehension.

The variable name l in the list comprehension is ambiguous and violates naming conventions as noted in previous reviews.

Apply the suggested fix from the previous review:

                            def makerec(
-                                lower, upper, dl, fc="none", ec="g", lw=1, ls="-"
+                                lower, upper, spacing, fc="none", ec="g", lw=1, ls="-"
                            ):
-                                origin = (lower[0] * dl[0], lower[1] * dl[1])
+                                origin = (lower[0] * spacing[0], lower[1] * spacing[1])
                                sizex, sizey = [
-                                    (u - l) * d for u, l, d in zip(upper, lower, dl)
+                                    (u - lower_val) * spacing_val 
+                                    for u, lower_val, spacing_val in zip(upper, lower, spacing)
                                ]

---

402-404: Error handling behavior change needs careful consideration.

As noted in the previous review, commenting out the error re-throwing fundamentally changes the test behavior from fail-fast to log-and-continue. While this provides better debugging information for the specific issue mentioned in the PR (copy before refinement causing mismatches), it could mask other legitimate test failures.

The previous suggestion to make this configurable is still valid:

-                        # if self.rethrow_:
-                        #     raise e
-                        # return diff_boxes(slice1, slice2, box)
+                        if getattr(self, 'rethrow_', True):
+                            raise e
+                        return diff_boxes(slice1, slice2, box)

This allows debugging mode when investigating the specific AMR refinement timing issues while preserving test integrity by default.

src/core/data/ndarray/ndarray_vector.hpp (1)

273-273: Minor: Remove extra empty line.

There's an unnecessary extra empty line that should be removed for consistency with the surrounding code formatting.

-

tests/simulator/test_advance.py (1)

290-291: Remove unused matplotlib.pyplot import.

The matplotlib.pyplot import is flagged as unused by static analysis, but examining the code shows it's actually used via plt in the exception handling block.

The static analysis tool missed the plt usage. However, consider moving the import closer to its usage for better code organization:

-                        import matplotlib.pyplot as plt
                        from matplotlib.patches import Rectangle
+                        import matplotlib.pyplot as plt

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

src/amr/physical_models/hybrid_model.hpp (1)

139-142: LGTM: Clear NaN handling with good documentation.

The explicit zeroing of state.J with detailed comments effectively prevents NaN propagation, particularly for the 1D case where Jx isn't updated by Ampère's law. The comment clearly explains the rationale and the placement after electromagnetic initialization is appropriate.

Consider whether this dimension-specific initialization could be moved to the state.J construction/initialization itself to avoid the initialize-then-zero pattern, but the current approach is clear and functional.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

tests/core/numerics/interpolator/test_main.cpp (2)

534-536: Simplify the zero initialization syntax.

The explicit zero initialization is good practice for ensuring deterministic test behavior. However, the syntax can be simplified.

Apply this diff to simplify the syntax:

-        (*(&rho)).zero();
-        (*(&rho_c)).zero();
-        v.zero();
+        rho.zero();
+        rho_c.zero();
+        v.zero();

---

710-712: Simplify the zero initialization syntax.

Same as the 1D version, the explicit zero initialization is good practice but the syntax can be simplified.

Apply this diff to simplify the syntax:

-        (*(&rho)).zero();
-        (*(&rho_c)).zero();
-        v.zero();
+        rho.zero();
+        rho_c.zero();
+        v.zero();

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (4)

33-33: Remove duplicate include.

The header amr/resources_manager/amr_utils.hpp is already included on line 18. This duplicate include should be removed.

-#include "amr/resources_manager/amr_utils.hpp"

---

282-284: Remove debug output and use proper logging.

This debug output should be removed or replaced with proper logging using the PHARE logging framework. Additionally, the language used is inappropriate for production code.

-            std::cout << "I am fucking regriding level " << levelNumber << " with oldLevel "
-                      << (oldLevel ? std::to_string(oldLevel->getLevelNumber()) : "null")
-                      << " at time " << std::setprecision(16) << initDataTime << "\n";
+            PHARE_LOG_LINE_STR("Regridding level " + std::to_string(levelNumber) 
+                               + " with oldLevel " + (oldLevel ? std::to_string(oldLevel->getLevelNumber()) : "null")
+                               + " at time " + std::to_string(initDataTime));

---

408-408: Remove debug output.

Replace std::cout with proper logging framework.

-            std::cout << "FILLING ELECTRIC GHOSTS\n";
+            PHARE_LOG_LINE_STR("Filling electric ghosts");

---

410-439: Fix critical component access bug and remove unused variables.

Multiple issues need to be addressed:

1. Critical Bug: All three electric field components are incorrectly accessing Component::X
2. Unused Variables: Ex, Ey, Ez variables are declared but never used
3. Code Logic: The NaN initialization logic is sound but the component access must be fixed first

                 auto& Ex    = E(core::Component::X);
-                auto& Ey    = E(core::Component::X);
-                auto& Ez    = E(core::Component::X);
+                auto& Ey    = E(core::Component::Y);
+                auto& Ez    = E(core::Component::Z);

Since these variables are unused, consider removing them entirely:

                 auto box    = patch->getBox();
                 auto _      = resourcesManager_->setOnPatch(*patch, E);
-                auto& Ex    = E(core::Component::X);
-                auto& Ey    = E(core::Component::X);
-                auto& Ez    = E(core::Component::X);
                 auto layout = layoutFromPatch<GridLayoutT>(*patch);

src/core/data/grid/grid.hpp (3)

51-58: Review template parameter design and NaN initialization.

The template parameter U = value_type with FloatingPoint<U> constraint seems redundant since U defaults to value_type. Consider simplifying to requires FloatingPoint<value_type>.

Also, verify that static_cast<U>(std::nan("")) is the intended approach for NaN initialization.

-    template<FloatingPoint U = value_type, std::size_t dim>
+    template<std::size_t dim>
+        requires FloatingPoint<value_type>
     Grid(std::string const& name, PhysicalQuantity qty, std::array<std::uint32_t, dim> const& dims,
-         value_type value = static_cast<U>(std::nan("")))
+         value_type value = std::numeric_limits<value_type>::quiet_NaN())

---

60-67: Consistent template parameter simplification needed.

Same issue as the previous constructor - the template parameter U is redundant. Also consider using std::numeric_limits for NaN generation for better type safety.

-    template<FloatingPoint U = value_type, typename GridLayout_t>
+    template<typename GridLayout_t>
+        requires FloatingPoint<value_type>
     Grid(std::string const& name, GridLayout_t const& layout, PhysicalQuantity qty,
-         value_type value = static_cast<U>(std::nan("")))
+         value_type value = std::numeric_limits<value_type>::quiet_NaN())

---

51-85: Potential constructor ambiguity concerns.

The constructor overload resolution relies on SFINAE with requires clauses. While this should work correctly, consider whether the split between floating-point and non-floating-point constructors could create maintenance issues or unexpected behavior for users.

Consider documenting this design decision in class comments to help future maintainers understand why constructors are split based on value type, and ensure that the initialization behavior is well-documented for library users.

src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)

365-365: Use const reference for better performance.

The hybridModel should be declared as a const reference since it's not being modified in this context.

-            auto& hybridModel = static_cast<HybridModel&>(model);
+            auto const& hybridModel = static_cast<HybridModel const&>(model);

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

tests/simulator/test_advance.py (3)

77-77: Cleanup registration is still disabled - resource leak risk remains.

This issue was previously flagged. Consider implementing the suggested configurable approach to allow selective cleanup disabling for debugging while preserving default cleanup behavior.

---

290-311: Fix ambiguous variable name in makerec function.

The variable l in the list comprehension violates naming conventions and reduces readability, as flagged by static analysis.

Apply this fix to improve readability:

                            def makerec(
-                                lower, upper, dl, fc="none", ec="g", lw=1, ls="-"
+                                lower, upper, spacing, fc="none", ec="g", lw=1, ls="-"
                            ):
-                                origin = (lower[0] * dl[0], lower[1] * dl[1])
+                                origin = (lower[0] * spacing[0], lower[1] * spacing[1])
                                sizex, sizey = [
-                                    (u - l) * d for u, l, d in zip(upper, lower, dl)
+                                    (u - lower_val) * spacing_val 
+                                    for u, lower_val, spacing_val in zip(upper, lower, spacing)
                                ]

---

402-404: Exception handling behavior change already flagged.

The commented out error re-throwing and diff_boxes return was previously identified as changing test behavior from fail-fast to log-and-continue. The suggested configurable approach should be implemented.

tests/simulator/test_advance.py (1)

290-385: Consider organizing the extensive debugging code into a helper method.

The debugging visualization code is valuable but makes the main test method quite lengthy. Consider extracting this into a separate helper method for better maintainability.

+    def _generate_overlap_failure_diagnostics(self, pd1, pd2, box, ilvl, slice1, slice2, failed_i, failed_j, loc_b1, loc_b2, datahier):
+        """Generate detailed plots and diagnostics for overlap assertion failures."""
+        import matplotlib.pyplot as plt
+        from matplotlib.patches import Rectangle
+        
+        def makerec(lower, upper, spacing, fc="none", ec="g", lw=1, ls="-"):
+            # ... (move the makerec function here)
+        
+        # ... (move all the plotting code here)
+        
     try:
         assert_fp_any_all_close(slice1, slice2, atol=5.5e-15, rtol=0)
         checks += 1
     except AssertionError as e:
-        # ... (replace with call to helper method)
+        if box.ndim == 2:
+            failed_i, failed_j = np.where(np.abs(slice1 - slice2) > 5.5e-15)
+            self._generate_overlap_failure_diagnostics(pd1, pd2, box, ilvl, slice1, slice2, failed_i, failed_j, loc_b1, loc_b2, datahier)

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro